review: ground a cluster member on its shared anchor, not only the evidence's vocabulary - #363
Conversation
…s shared anchor, not only the evidence's vocabulary Run 32390393344 (webapp#41609) posted one finding twice: the clusterer correctly proposed the same-line cross-source pair as one cluster and the grounding tripwire vetoed it, because the evidence spoke in the hunk's identifiers while both claims spoke config-side, zero shared salient tokens. The evidence is model prose with free word choice, so the check graded the clusterer's phrasing rather than the identity it asserted. An exactly shared anchor (member on the survivor's own line) now grounds a proposed member with no vocabulary needed, and salient tokens fold casing styles so pre_flight_moderation_check and PreFlightModerationCheck read as one token. Survivor-text grounding was considered and rejected; run 30587343777's pinned fixtures show the false merge it would cause. The 41609 pair replays verbatim as a regression fixture and merges to one comment.
🦋 Changeset detectedLatest commit: 899519e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus 0441b7c6a581 (10 cases).
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
sxkosone
left a comment
sxkosone
left a comment
There was a problem hiding this comment.
Approving these to keep review bot work ongoing, but haven't read the PR description or the code
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (structural !== undefined) { | ||
| return structural; | ||
| } | ||
| if (member.line !== undefined && member.line === survivor.line) { |
There was a problem hiding this comment.
question (non-blocking): The anchor path grounds on colocation, not identity — it retires the tripwire exactly where distinct-defect collisions are densest. isSalientToken's doc (dedup-cluster.ts:73) states the asymmetry this tier is calibrated on: "a missed merge costs a duplicate comment while a wrong one drops a reviewer's distinct finding" — yet same path + same line + proposed-by-the-model is now sufficient, and the offered safeguard ("only claims the model PROPOSED reach this check") is the very assertion the tripwire existed to second-guess; the sibling test "keeps the run's neighbours on the same lines out of the cluster" exists because distinct defects routinely share a line, and the pinned staleAfter counterexample only survives because it sits at :11 rather than :8. It's also worth asking about ordering: the PR defers the 90-day audit of "ungrounded" rejections to a follow-up, but that audit is the evidence that would say whether one measured false veto justifies retiring the check for same-line members, or whether a narrower fix (e.g. requiring the evidence to be groundable at all before the anchor shortcut applies) covers run 32390393344 without the blast radius.
review details
found by first-principles…h's cost Folds in the non-blocking review feedback on #363: - each clusterer-absorbed copy in dispatch-result.json carries groundedBy: "anchor" | "evidence", so the planned audit of "ungrounded" rejections can tell the two paths apart - a new test pins the anchor path's accepted cost: a distinct non-blocking finding on the survivor's exact line folds in when the model proposes it (documentation-2 re-anchored to :8) - dedup.ts's determinism-boundary header no longer states the vocabulary check as unconditional - the clusterer prompt says "normally discarded" instead of promising a mechanical discard the code stopped making - sharesSalientToken is unexported (this PR removed its last importer) - the empty-canonical guard gets a comment and a test (an all-underscore token folds to "" and would ground any two claims) - the runClaims fixture moves to module scope, matching its siblings
|
Folded the review feedback in with 1534d1d (threaded replies are blocked for my token, so batching here):
Not taken: a merge-level test where the casing fold alone grounds a member; the fold stays pinned at the salientTokens level. |
Guidance for reviewersTriage notes for reviewers: risky files by owning team, repeated changes, and files excluded from review. github-actions (3 files)
Common patterns3 files: Adding a - {index: number; via?: "clusterer"}[]
+ {
+ index: number;
+ via?: "clusterer";
+ groundedBy?: "anchor" | "evidence";
+ }[]- via: "clusterer",
+ via: "clusterer",
+ groundedBy: "evidence",review detailsreview-v1.17.1 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation |
There was a problem hiding this comment.
8 of 10 prior review threads resolved; 2 still unaddressed as of 1534d1d:
2 non-blocking threads still open
- question (non-blocking)
workflows/review/lib/dedup-cluster.ts:211: The anchor path grounds on colocation, not identity — it retires the tripwire exactly where distinct-defect collisions a... - thought (non-blocking)
workflows/review/lib/dedup-cluster.ts:91: The casing fold is motivated by run 32390393344 but would not have fixed it, and nothing pins its effect on a merge. Gro...
review details
review-v1.17.1 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation| into.push({ | ||
| index, | ||
| via: "clusterer", | ||
| groundedBy: |
There was a problem hiding this comment.
suggestion (non-blocking): groundedBy re-derives the grounding decision instead of receiving it from clusterMemberRejection, so the two can silently disagree. The two expressions are identical today, so this is not a live bug — but the field's stated purpose (its JSDoc at dedup.ts:132-139, and the PR description's follow-up) is to feed an audit that decides whether the vocabulary tripwire earns its keep, and the only thing keeping the record honest is the // Mirrors the first grounding test in clusterMemberRejection comment two lines above. A duplicated predicate guarding a decision about deleting a safety check is worth collapsing to one source of truth: have the rejection helper return which path admitted the member, and let the caller record what the check actually decided rather than re-deciding it.
A sketch, not a committable replacement:
// in dedup-cluster.ts
export const clusterMemberAdmission = (
survivor: Claim,
member: Claim,
evidenceTokens: ReadonlySet<string>,
):
| {reason: ClusterRejection["reason"]}
| {reason?: undefined; groundedBy: "anchor" | "evidence"} => {
const structural = structuralRejection(survivor, member);
if (structural !== undefined) {
return {reason: structural};
}
if (member.line !== undefined && member.line === survivor.line) {
return {groundedBy: "anchor"};
}
const evidenceUsable =
evidenceTokens.size > 0 && sharesSalientToken(evidenceTokens, survivor);
return evidenceUsable && sharesSalientToken(evidenceTokens, member)
? {groundedBy: "evidence"}
: {reason: "ungrounded"};
};
// in dedup.ts, replacing the reason/push pair
const admission = clusterMemberAdmission(
survivor,
claims[index],
evidenceTokens,
);
if (admission.reason !== undefined) {
for (const id of namedByHead.get(index) ?? []) {
clusterRejections.push({id, reason: admission.reason});
}
continue;
}
into.push({index, via: "clusterer", groundedBy: admission.groundedBy});
review details
found by correctness-reviewer | also flagged by skill-auditor (out-of-lane); holistic; conventions (at line 856): Anchor rule re-derived inline in dedup.ts instead of being read from dedup-cluster.ts.| * must exist, the paths and sources must satisfy the same constraints tier 1 | ||
| * enforces, the model's own grounding evidence must appear in every member's | ||
| * text, and only a NON-BLOCKING copy may be absorbed on a model's word | ||
| * text unless the member sits on the survivor's exact line, and only a |
There was a problem hiding this comment.
suggestion (non-blocking): The change updates three statements of the grounding rule but leaves dedup.ts's canonical "the line does nothing in tier 2" paragraph contradicting it. The diff carefully amends the determinism-boundary paragraph (dedup.ts:91), structuralRejection's "deliberately absent" note, and clusterMemberRejection's docblock — but dedup.ts:68-73, the "unit of identity, restated" paragraph that is the module's most-quoted statement of why tier 2 exists, still asserts the line carries no weight in tier 2, which is now the opposite of what clusterMemberRejection does first. In a module whose headers are the design record and are cited by run number, the one paragraph a reader treats as canonical is the one left stale.
A sketch, not a committable replacement:
// dedup.ts, the "unit of identity, restated" paragraph (~:72):
* pairs that also clear its looser text floor). The line is never REQUIRED in
* tier 2; it counts only the other way round, as evidence — inside tier 1 as a
* lower text floor, and in tier 2 as the one grounding a model's proposal needs
* no vocabulary for (see {`@link` clusterMemberRejection}) — plus as the
* survivor's posting anchor.
review details
found by holistic…sing fold, and the anchor against a tier-1 head Two tests answering the open review threads on #363, no behavior change: - The casing fold was exercised only as a salientTokens unit assertion, and the motivating run would not have needed it (its two spellings live in the two claims, and grounding compares the evidence against each claim). A new test pins a merge outcome on the fold: a cross-line member whose claim spells the evidence's config key in the other casing convention merges on the vocabulary path, and is rejected ungrounded without the fold. - Nothing pinned the anchor path against a survivor the clusterer never named. A new test holds the transitivity chain: tier 1 bridges the named member into a blocking head on the same line, and the proposed member grounds on the anchor against that head even though the head never names the evidence (the survivor-end veto the vocabulary path would have made).
There was a problem hiding this comment.
Approved — no blocking issues found.
3 of 6 prior review threads resolved; 3 still unaddressed as of 899519e:
3 non-blocking threads still open (1 previously reported)
- suggestion (non-blocking)
workflows/review/lib/dedup.ts:855:groundedByre-derives the grounding decision instead of receiving it fromclusterMemberRejection, so the two can sil... - suggestion (non-blocking)
workflows/review/lib/dedup.ts:91: The change updates three statements of the grounding rule but leaves dedup.ts's canonical "the line does nothing in tier... - question (non-blocking)
workflows/review/lib/dedup-cluster.ts:211
Non-blocking observations (5)
workflows/review/lib/dedup-cluster-grounding.test.ts:129suggestion (non-blocking): The anchor grounding rule is decided indedup-cluster.tsbut re-derived indedup.tsto label it. (holistic)workflows/review/lib/dedup-cluster-grounding.test.ts:302note (non-blocking): The anchor path's stated bound ("only claims the model PROPOSED reach this check") does not survive tier-1 head substitution at the member end. (holistic)workflows/review/lib/dedup-cluster-grounding.test.ts:25suggestion (non-blocking, documentation): Fixture provenance names a version the run did not use. (documentation)workflows/review/lib/dedup-cluster-grounding.test.ts:298question (non-blocking): When the survivor is a tier-1 head, the shared anchor is co-location with a claim the model never named, not with the one it did. (first-principles)workflows/review/lib/dedup-cluster-grounding.test.ts:37nitpick (non-blocking): Fixture factory is named for its shape, not the defect it carries, unlike the two files this one says it mirrors. (conventions)
review details
review-v1.17.1 | schema 2 | depth scoped | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation
🖍 _This is an audit!_ 🖍 This repo's installed reviewer was on review-v1.17.1; 1.18.0 is cut and carries the prose judge (#362), whose rollout wants a week of live artifacts to watch the judge's error rate, so let's just move our own install first. Also in the hop: shared-anchor grounding with casing-folded salient tokens (#363), staged jira ticket context (#361, optional config we don't set here; staging degrades to available:false), and the turn-cap/timeout raises (#369). Mechanics, per the hand-merge flow (not `gh aw update`): 3-way merge of the installed review.md from 1.17.1 to 1.18.0, one conflict where the new lib-checkout ref landed against the LOCAL OVERRIDE comment, resolved by keeping the override and taking the new ref; `source:` and the checkout `ref:` moved to review-v1.18.0 together; recompiled with gh-aw v0.85.4 (same compiler as the current lock); the compile's merge=ours strip on .gitattributes reverted. All 6 KHAN/ACTIONS LOCAL OVERRIDE blocks survived, and review-pins.test.ts is green locally (5 tests, including the content guard diffing the install against the pinned source). [KORE-2512](https://khanacademy.atlassian.net/browse/KORE-2512) [KORE-2512]: https://khanacademy.atlassian.net/browse/KORE-2512?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Author: jwbron Auditors: jeresig, jaredly Required Reviewers: Approved By: Checks: ⌛ 1 check is pending, ⏭️ 8 checks have been skipped, ✅ 1 check was successful, ❌ 1 check failed Pull Request URL: #373
3 participants
Run 32390393344 (webapp#41609, review-v1.14.0 install, lib at 1.17.0) posted one finding twice: two comments in one run at 16:28:08 on moderation_helpers.go:31, one labeled thought and one question, both carrying the same claim (the experiment enrolled 3 configs, the change flips ~112). The author's reply was "broader rollout is intended and safe, stop raising this."
The artifact says the clusterer did its job: it proposed exactly one cluster with both ids. The merge died in the grounding tripwire, which requires the member's text to share a salient code token with the cluster's evidence. The evidence spoke in the hunk's identifiers (
_configIncludesModeration,shouldModerateDuringMainCompletion) while both claims spoke config-side (pre_flight_moderation_check,config_files, 112/151), zero shared tokens, sofirst-principles-1was rejected "ungrounded". The evidence is model prose with free word choice, so the check was grading the clusterer's phrasing rather than the identity it asserted.Two changes:
PreFlightModerationCheckandpre_flight_moderation_checkread as one token), so the vocabulary path tests names, not spellings.Considered and rejected: grounding the member against the survivor's own claim text. The suite already holds the counterexample; run 30587343777's cap survivor names
staleAfterin a while-here aside, and the distinct staleAfter finding (documentation-2) would falsely ground against it. That rejection stays pinned.Two existing tests changed meaning deliberately. The inert-evidence test now splits: the two members on the survivor's exact line merge on the anchor (the run's own autofix discharged all four asks with one rewritten comment, so that's the true outcome), while the line-9 member still needs the evidence and stays rejected. The no-subtraction test moves its thin-worded copy one line off so it keeps exercising the ungrounded path it exists to pin. The 41609 pair replays verbatim in a new dedup-cluster-grounding.test.ts (the old file hit the 1000-line cap, same split precedent as dedup.test.ts before it) and merges to one comment.
Follow-up, tracked separately: audit the 90-day window of dispatch-result artifacts for "ungrounded" rejections to get the before/after rate, and decide whether the vocabulary path earns its keep at all.
Review feedback folded in (1534d1d): merge records now say which path grounded each clusterer-absorbed copy (
groundedBy: "anchor" | "evidence"), so that audit is answerable from the artifacts; a new test pins the anchor path's accepted cost (a distinct non-blocking finding on the survivor's exact line folds in when the model proposes it); and the clusterer prompt says "normally discarded" instead of promising the mechanical discard the code stopped making.KORE-2509